New arrow C-API implementation #50
Conversation
|
Merge after: duckdb/duckdb-go-bindings#39 |
|
@taniabogatsch FYI |
|
Thanks! Will have a look once we've released the bindings with the latest changes. 👍 |
|
@taniabogatsch Could you take a look this PR? What the sequence to merge it, should I the arrowmapping version increase in the duckdb package before merge or split pr to 2 - arrowmapping changes and the changes in the duckdb package? |
|
I think your duckdb-go-bindings changes should already be on |
|
We pushed a release today. |
|
Yes, I sow. I mean arrowmapping package in this repo. |
|
ah, I see now! We likely need to bump it in a separate PR - i.e., first one with the new functions in the arrow mapping package, then I can bump that version. 👍 |
|
@taniabogatsch Very strange with tests when the go version 1.24 is specified in the go.mod, the tests are failed. I have created an issue to update the go version to 1.25 in the CI and packages. #63 |
|
@taniabogatsch I revert transitive dependency versions. The failed tests was fixed |
|
@taniabogatsch Will you merge it? |
|
Yes, but I'll need to review it first. 👍 Didn't find the time to take a look yet, will so next week. |
taniabogatsch
left a comment
taniabogatsch
left a comment
There was a problem hiding this comment.
Thanks for the PR / implementation - I saw that its not a huge PR, so I squeezed in a review already ;)
arrow.go
Outdated
| } | ||
| if arrowmapping.ArrowScan(a.conn.conn, name, arrowStream) == mapping.StateError { | ||
| release() | ||
| return nil, errors.New("duckdb_arrow_scan") |
There was a problem hiding this comment.
Can we turn this into a bit more elaborate error? I.e., that something went wrong in the DuckDB-side? Also, can we trigger this in a test? If not, should we tell people to file a bug report if they run into it?
There was a problem hiding this comment.
Ok, but this part of the code part is unchanged and here is no method to get the real error in the ArrowScan (I think it should be deprecated in the future). I have added some more informative err message.
I want to add the new type table function - ArrowTableUDF, that will use the new C-API, and deprecated this method. With this new UDF and replacement scan we can provide the same functionality to have arrow view.
The main difference will be in the options to delete the view definition after using. Do you know, the C-API will contain the method to deregister replacement scans and UDFs?
There was a problem hiding this comment.
Ah - for replacement scans I don't think its possible to deregister (alter) yet.
For table functions it should be possible (only checked latest main)
duckdb_state duckdb_register_table_function(duckdb_connection connection, duckdb_table_function function) {
// ...
try {
con->context->RunFunctionInTransaction([&]() {
auto &catalog = duckdb::Catalog::GetSystemCatalog(*con->context);
duckdb::CreateTableFunctionInfo tf_info(tf);
tf_info.on_conflict = duckdb::OnCreateConflict::ALTER_ON_CONFLICT;
catalog.CreateTableFunction(*con->context, tf_info);
});
} catch (...) { // LCOV_EXCL_START
return DuckDBError;
} // LCOV_EXCL_STOP
return DuckDBSuccess;
}
taniabogatsch
added
feature / enhancement
taniabogatsch
added
the
changes requested
|
The PR just replaces the internal implementation of the Arrow.QueryContext method and record reader implementation. I also rename the reader to make it simple. |
|
Thanks! |
|
Thanks, will you push the new tag for the duckdb package? |
2 participants
Rewrite the Arrow.QueryContext method to use new DuckDB Arrow C-API
Closes #5